Issue 6091: Remove Facebook collections library #6129

rtjd6554 · 2025-11-27T14:32:43Z

Make sure you have checked all steps below.

Issue

My PR fully resolves the following issues. I've referenced an issue in the PR title, for example "Issue 1234 - My
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
- Resolves Remove Facebook collections library #6091

Tests

My PR adds the following tests based on our test strategy OR does not need testing for this extremely good reason:
- Existing execution of test suite
- New tests for classes created: ByteArrayTest

Documentation

In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
separate issue for that below.
If I have added new Java code, I have added Javadoc that explains it following our conventions and style.
If I have added or removed any dependencies from the project, I have updated the NOTICES file.

m09526 · 2025-11-28T09:21:47Z

Could we replace this with a commons lang3 Pair? They have ImmutablePair and MutablePair as required.

patchwork01 · 2025-11-28T09:48:56Z

Personally I'd rather avoid using a Pair-type data structure at all, in favour of something with a more descriptive name. Using a Java record lets you achieve that with very little boilerplate, something like this:

public record FieldValue<T>(String fieldName, T value) {
}

I'd keep that in the Athena code, next to where it's used, maybe nest it in the class and make it static.

…n of class

ca61688 · 2025-12-02T16:05:20Z

java/athena/src/main/java/sleeper/athena/record/FieldAtDimension.java

+ */
+package sleeper.athena.record;
+
+public record FieldAtDimension(int dimension, Object value) {


This is field AT dimension whereas other is field AS String. Is that correct?

Addressed typo

ca61688 · 2025-12-02T16:08:16Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+     *
+     * @param  array1 first ByteArray object
+     * @param  array2 second ByteArray object
+     * @return        true is equal


true if* equal

Addressed typo

ca61688 · 2025-12-02T16:08:53Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+     * Method for checking equality versus generic object declaration.
+     *
+     * @param  o object to compare versus
+     * @return   true is equal


true if* equal

Addressed typo

ca61688 · 2025-12-02T16:11:10Z

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ByteArrayTest {


Is it worth testing equals validation for one null one not and for the contents of the arrays being equal?

ca61688

Few small comments otherwise looks good

gaffer01 · 2025-12-03T09:54:33Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+    }
+
+    /**
+     * Method for checking equaility.


Typo on equaility - should be equality

patchwork01

Facebook have the same license as this project. I'm basing this review on my own reading of the terms:

https://github.com/facebookarchive/jcommon/blob/master/LICENSE

NOTICES

java/core/src/main/java/sleeper/core/util/ByteArray.java

patchwork01 · 2025-12-03T09:26:32Z

java/core/src/main/java/sleeper/core/util/ByteArray.java

+
+/**
+ * Class to provide wrapper object for primitive byte array into an object.
+ * Require as hashCode and equals requied for usage of type within a HashSet.


Please see the Oracle Javadoc style guide:

https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html#styleguide

A class description shouldn't start with "class", see "Class/interface/field descriptions can omit the subject and simply state the object." If we take the first sentence as it is, it could start with "A wrapper for ...".

There's a typo at "requied".

There are grammatical problems at "for primitive", "Require as", "equals required", "usage of type".

It's not technically true that by wrapping a byte array we turn it into an object. A byte array is already an object.

I don't think HashSet is relevant to this class?

I think the important part for us is it implements Comparable and equals/hashCode.

Addressed,

disagree on the HashSet comment as otherwise we could just use the primitive here, it is directly because we use in within HashSets that we need it as a separate class.

I don't know what you mean, we don't use it in HashSets.

java/core/src/main/java/sleeper/core/util/ByteArray.java

java/core/src/main/java/sleeper/core/schema/type/ByteArray.java

java/core/src/main/java/sleeper/core/util/ByteArray.java

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

patchwork01 · 2025-12-03T10:25:16Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+    }
+
+    @Test
+    void shouldValidateEqualsMethodWithNullObjects() {


A test name should describe the behaviour being asserted, rather than name the method being tested.

patchwork01 · 2025-12-03T10:25:53Z

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

+        ByteArray valid = ByteArray.wrap(new byte[]{'a', 2, 3});
+
+        // When / Then
+        assertThat(ByteArray.equals(valid, null)).isFalse();


It looks like we need a version of these tests for the instance method as well?

I don't think we ever use ByteArray.equals(ByteArray, ByteArray), so it might be simpler to get rid of that and only keep the instance method?

See my comment on ByteArray.equals(ByteArray, ByteArray).

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

patchwork01 · 2025-12-04T14:05:36Z

NOTICES



+The file sleeper.core.util.ByteArray.java contains code that is heavily based on ByteArray from the Facebook Jcommon
+collections library, licensed under the Apache License, Version 2.0.


In the paragraph below we've used a relative file system path. It might be worth being consistent with that, rather than using the package name.

I think the library name is capitalised "Facebook JCommon Collections", although the only place I could find it not in all lower case is here:
https://mvnrepository.com/artifact/com.facebook.jcommon/collections

patchwork01 · 2025-12-04T14:06:56Z

java/athena/src/main/java/sleeper/athena/record/FieldAsString.java

+package sleeper.athena.record;
+
+/**
+ * Storage of key pairing for field and value.


I don't know what "key pairing" means here. It's a value for a field, held as a string along with the name of the field.

patchwork01 · 2025-12-04T14:08:59Z

java/athena/src/main/java/sleeper/athena/record/FieldAtDimension.java

+package sleeper.athena.record;
+
+/**
+ * Storage of key pairing of dimension and value.


I don't know what "key pairing" means here. It's a value for a row key field, held along with the dimension of the row key, which is its index in the list of row keys in the schema. I'd include an explanation of what a dimension is here.

patchwork01 · 2025-12-04T14:11:42Z

java/athena/src/main/java/sleeper/athena/record/FieldAsString.java

+ * Storage of key pairing for field and value.
+ *
+ * @param fieldName field name
+ * @param value     value stored


I think it reads a bit oddly to omit "the" in these phrases. I think "value stored" is a bit vague, given it's not the actual value in the format we use elsewhere.

I'd say "the field name" and "a string representation of the value".

patchwork01 · 2025-12-04T14:14:16Z

java/athena/src/main/java/sleeper/athena/record/FieldAtDimension.java

+ * Storage of key pairing of dimension and value.
+ *
+ * @param dimension dimension key
+ * @param value     value stored


I think it reads a bit oddly to omit "the" in these phrases. I don't know what "key" means in "dimension key".

I'd say "the index of the row key in the schema" and "the value".

patchwork01 · 2025-12-04T14:56:44Z

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

+        ByteArray valid = ByteArray.wrap(new byte[]{3, 'd', 5});
+
+        // When / Then
+        assertThat(valid.equals(ByteArray.wrap(new byte[]{3, 'd', 5}))).isTrue();


This seems to duplicate the test above, shouldMatchByteArraysWhenWrappedUsingSameData.

patchwork01 · 2025-12-04T14:59:15Z

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

+    @Test
+    void shouldCorrectlySortByteArrays() {
+        // Given
+        ByteArray first = ByteArray.wrap(new byte[]{1, 2, 3});
+        ByteArray second = ByteArray.wrap(new byte[]{4, 5, 6, 7, 8});
+
+        // When / Then
+        assertThat(first).isLessThan(second);


The test name suggests this is testing more than it is. We need more than this one case to test sorting.

It'd be good to make nested test classes for equality and for sorting.

patchwork01 · 2025-12-04T15:00:12Z

java/core/src/test/java/sleeper/core/util/ByteArrayTest.java

+
+        // Then 2
+        assertThat(treeSet).containsExactly(first);
+        assertThat(treeSet).doesNotContain(second);


The containsExactly assertion already asserts that it doesn't contain the second byte array.

patchwork01 · 2025-12-04T15:03:33Z

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ByteArrayTest {


Please could you establish a consistent naming scheme for tests that do similar things?

It's a bit difficult to see the intention right now. Nested test classes could help as well.

For example, there's shouldFindByteArraysAreEqualWithSameSourceArray (my wording) and shouldMatchByteArraysWhenWrappedUsingSameData. These tests are extremely similar, but it's hard to tell because the names have quite different wording.

patchwork01 · 2025-12-04T15:14:18Z

java/core/src/test/java/sleeper/core/schema/type/ByteArrayTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ByteArrayTest {


There are still a number of tests that use vague names as in my previous comments (e.g. #6129 (comment)).

Several tests use "shouldValidate..." when they're not about validation.

The "shouldReturnCorrectComparison" and "shouldProvideCorrectComparison" don't explain what correct means, or which behaviour you wanted to assert.

6091: New pair class to remove facebook.collections usage

630e219

patchwork01 and others added 13 commits November 28, 2025 09:58

6091 Remove Pair

f6bb550

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

0f9c02e

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

f09aec3

6091: Move records out to seperate class files.

da1f344

6091: New Class for ByteArray

972cafa

6091: Adjust imports to replace facebook collections with core versio…

4b26524

…n of class

6091: Update test class

0bae30e

6091: Update pom declarations

0fab329

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

35524cf

6091: Update NOTICES

e2612aa

6091: Improve comparator

95d060b

6091: Access modifiers correction

d9f2036

6091: Update compare logic

73d4c64

rtjd6554 marked this pull request as ready for review December 2, 2025 15:34

rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 2, 2025

ca61688 self-assigned this Dec 2, 2025

ca61688 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 2, 2025

ca61688 reviewed Dec 2, 2025

View reviewed changes

ca61688 requested changes Dec 2, 2025

View reviewed changes

ca61688 assigned rtjd6554 and unassigned ca61688 Dec 2, 2025

rtjd6554 added 3 commits December 3, 2025 08:07

6091: Correct class name typo

46b7450

6091: Correct java doc typo

d3cd782

6091: Additional unit test

6fa837a

ca61688 approved these changes Dec 3, 2025

View reviewed changes

ca61688 assigned rtjd6554 and unassigned ca61688 Dec 3, 2025

patchwork01 added the on-hold label Dec 3, 2025

6091: Update copyright declaration

1e019bb

gaffer01 reviewed Dec 3, 2025

View reviewed changes

patchwork01 requested changes Dec 3, 2025

View reviewed changes

rtjd6554 added 10 commits December 3, 2025 10:45

6091: Typos and Javadoc corrections

1dc5db4

6091: Rename class and Javadoc

1e9e865

6091: MIssing Javadoc

2fb1a8f

6091: Javadoc improvements

764cbbf

6091: Update unit tests

8729321

6091: Checkstyle updates

837a218

6091: Update copyright

87aa5c2

6091: Update copyright

e1cc495

6091: Update copyright

5908008

6091: Simplfy copyright

4048f0c

patchwork01 removed the on-hold label Dec 3, 2025

rtjd6554 and others added 4 commits December 3, 2025 13:03

Merge branch 'develop' into 6091-Remove-Facebook-Collections-Library

fc6e5d0

6091: Refactor nested classes

90047a1

6091: Checkstyle updates

5a4faee

6091: Spotbugs adjustments

615dabf

rtjd6554 removed their assignment Dec 4, 2025

rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 4, 2025

6091: Update copyright and NOTICES

32da748

patchwork01 reviewed Dec 4, 2025

View reviewed changes

patchwork01 assigned rtjd6554 Dec 4, 2025

6091: Review comment updates

f3a2f77

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Dec 4, 2025


		import static org.assertj.core.api.Assertions.assertThat;

		public class ByteArrayTest {



		The file sleeper.core.util.ByteArray.java contains code that is heavily based on ByteArray from the Facebook Jcommon
		collections library, licensed under the Apache License, Version 2.0.

Issue 6091: Remove Facebook collections library #6129

Are you sure you want to change the base?

Issue 6091: Remove Facebook collections library #6129

Uh oh!

Conversation

rtjd6554 commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tests

Documentation

Uh oh!

m09526 commented Nov 28, 2025

Uh oh!

patchwork01 commented Nov 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ca61688 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtjd6554 commented Nov 27, 2025 •

edited

Loading